Conversation
|
We detected some changes in |
frandiox
left a comment
frandiox
left a comment
There was a problem hiding this comment.
Great job Ben! 🔥
This is far better commented than any of us devs do 😂
I lack context on the Image component but here are some minor comments about the code.
Also, wondering if this component should go in hydrogen-react instead?
|
FYI @frehner, Ben and I spoke about moving this to RSK, cause there's nothing unique to Remix in it's implementation. |
frehner
left a comment
frehner
left a comment
There was a problem hiding this comment.
This PR may also be relevant here Shopify/hydrogen-react#103
In any case, this looks pretty good. I used responsive image lint on the example page and it brought up two issues:
I don't think that's too bad of an issue, it just means that the sizes didn't match perfectly for a 1080p screen, right? I could be wrong on that, though.
This one does seem to be a potential bug/issue we would want to look into, though. I think.
There's a lot of helper functions here, which I like. It should help ensure that you can write pretty specific tests for each one which should help raise confidence on ensuring that things are all working as expected.
It may also be helpful to go through the existing tests and see if any of them should still apply; would help us prevent regressions on edge cases that we've fixed in the past.
Looking good! 👍
| ? undefined | ||
| : generateSizes(widths, fixedAspectRatio, crop); | ||
|
|
||
| return React.createElement(Component, { |
There was a problem hiding this comment.
I don't think you need React.createElement here, and instead can just use normal JSX with <Component/>
There was a problem hiding this comment.
Oh thanks — Bret actually did that part so I was just leaning on that. I can change it back to JSX though — is there a reason you did that @blittle?
| src, | ||
| /* | ||
| * Supports third party loaders, which are expected to provide | ||
| * a function that can generate a URL string |
There was a problem hiding this comment.
Is this signature (src, width, height, crop) => src for these functions standard or something you are coming up with here.
There was a problem hiding this comment.
Standard in how Imagery works — there are even more parameters you can pass, but felt overkill. One alternative would be to change this to params which would be an object of the same options.
That might be beneficial because crop could eventually be expanded to accept an object — which Imagery accommodates.
|
Closed in favour of moving to |
5 participants
Replaced by Shopify/hydrogen-react#114
Demo Link: https://n5jyshg6a-f1f4fa724b7467f41f07.myshopify.dev/image-demo?shopify_employee_access=true
<Image />now supports all unit types, both for responsive and fixed with images.Todo:
Picture Element
Picture component should look something like:
When inside the![]()
component should render a element, the last ![]()
component should render an ![]()
element. Ideally we'd somehow do that automatically — looking at the nodes of children, and then setting the
asprop for you. Alternatively, our component could re-export the Image component but as a<Source />component — which would be in keeping with HTML semantics.